Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark previously deprecated SSL settings as obsolete #183

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

robbavey
Copy link
Contributor

@robbavey robbavey commented Dec 2, 2024

  • SSL settings that were marked deprecated in version 3.15.0 are now marked obsolete, and will prevent the plugin from starting.
  • These settings are:
    • ca_file, which should be replaced by ssl_certificate_authorities
    • keystore, which should be replaced by ssl_keystore_path
    • keystore_password, which should be replaced by ssl_keystore_password
    • keystore_type, which should be replaced by ssl_keystore_password
    • ssl, which should be replaced by ssl_enabled

Relates: #179

- SSL settings that were marked deprecated in version `3.15.0` are now marked obsolete, and will prevent the plugin from starting.
- These settings are:
  - `ca_file`, which should be replaced by `ssl_certificate_authorities`
  - `keystore`, which should be replaced by `ssl_keystore_path`
  - `keystore_password`, which should be replaced by `ssl_keystore_password`
  - `keystore_type`, which should be replaced by `ssl_keystore_password`
  - `ssl`, which should be replaced by `ssl_enabled`
CHANGELOG.md Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setup of ssl parameters seems a bit complex now:

  def setup_ssl_params!
    # Infer the value if neither the deprecate `ssl` and `ssl_enabled` were set
    infer_ssl_enabled_from_hosts
  end

  def infer_ssl_enabled_from_hosts
    return if original_params.include?('ssl_enabled')

    @ssl_enabled = params['ssl_enabled'] = effectively_ssl?
  end

  def effectively_ssl?
    return true if @ssl_enabled

    hosts = Array(@hosts)
    return false if hosts.nil? || hosts.empty?

    hosts.all? { |host| host && host.to_s.start_with?("https") }
  end

I think that boils down to just:

def setup_ssl!
  return if original_params.include?('ssl_enabled')
  
  @ssl_enabled = if @ssl_enabled
    true
  else
    Array(@hosts).all? { |host| host && host.to_s.start_with?("https") }
  end
  
  params['ssl_enabled'] = @ssl_enabled
end

Though i'm not entirely sure what mutating params does in this. In general the params scope is kind of a mystery to me 😅

@robbavey
Copy link
Contributor Author

This was about as simple as I could get to:

  def setup_ssl_params!
    # Infer the value if neither `ssl_enabled` was not set
    return if original_params.include?('ssl_enabled')
    params['ssl_enabled'] = @ssl_enabled ||= Array(@hosts).all? { |host| host && host.to_s.start_with?("https") }
  end

Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@robbavey
Copy link
Contributor Author

Over to @karenzone for doc review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants